Protect uncaught_exceptions virtual table against the crash path - #5028
Closed
frankgh wants to merge 1 commit into
Closed
Protect uncaught_exceptions virtual table against the crash path#5028frankgh wants to merge 1 commit into
frankgh wants to merge 1 commit into
Conversation
Fixes three issues in ExceptionsTable, which records every uncaught exception from JVMStabilityInspector's central stability path: - Guard the persist(t) call so a throw can't skip inspectThrowable's disk_failure_policy / OOM die handling. - Drain preInitialisationBuffer under its monitor in flush() to avoid a ConcurrentModificationException that could abort node startup. - Bound preInitialisationBuffer so a startup exception storm (or a tool context that never flushes) can't retain stack traces without limit. patch by Francisco Guerrero; reviewed by TBD for CASSANDRA-21578
frankgh
commented
Aug 14, 2026
| // otherwise add() during iteration and trigger a ConcurrentModificationException, which would propagate out of | ||
| // setupVirtualKeyspaces() and abort node startup. | ||
| List<ExceptionRow> drained; | ||
| synchronized (preInitialisationBuffer) |
Contributor
Author
There was a problem hiding this comment.
iteration must be synchronized for arrays wrapped with Collections.synchronizedList. Here's an excerpt from the Collections.synchronizedList javadoc:
* It is imperative that the user manually synchronize on the returned
* list when traversing it via {@link Iterator}, {@link Spliterator}
* or {@link Stream}:
* <pre>
* List list = Collections.synchronizedList(new ArrayList());
* ...
* synchronized (list) {
* Iterator i = list.iterator(); // Must be in synchronized block
* while (i.hasNext())
* foo(i.next());
* }
* </pre>
frankgh
commented
Aug 14, 2026
| // together under the list monitor so concurrent persist() calls cannot race past the cap. | ||
| synchronized (preInitialisationBuffer) | ||
| { | ||
| if (preInitialisationBuffer.size() < PRE_INITIALISATION_BUFFER_CAPACITY) |
Contributor
Author
There was a problem hiding this comment.
we want to avoid this growing bigger than the 1000 limit we had set above in the ctor in the original patch
frankgh
commented
Aug 14, 2026
| ExceptionsTable.persist(t); | ||
| // Recording the exception for observability must never preempt the stability handling below (the | ||
| // disk_failure_policy / OOM "die" actions in inspectThrowable). Guard it like the StorageMetrics increment above. | ||
| try { ExceptionsTable.persist(t); } catch (Throwable ignore) { /* observability only, must not throw here */ } |
Contributor
Author
There was a problem hiding this comment.
we should never allow a failure here prevent us from processing the exception, we follow the same pattern as line 74
smiklosovic
approved these changes
Aug 15, 2026
smiklosovic
left a comment
smiklosovic
left a comment
Contributor
There was a problem hiding this comment.
thanks for making this more robust
Contributor
Author
|
Closed via 1d29b33 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes three issues in ExceptionsTable, which records every uncaught exception from JVMStabilityInspector's central stability path:
patch by Francisco Guerrero; reviewed by TBD for CASSANDRA-21578